-
Notifications
You must be signed in to change notification settings - Fork 469
Added databricks_permission resource for managing permissions for individual principals.
#5161
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…ndividual principals. This resource provides fine-grained control over permissions by managing a single principal's access to a single object, unlike `databricks_permissions` which manages all principals' access to an object at once. This is particularly useful for: - Managing permissions for different teams independently - Token and password authorization permissions that previously required all principals in one resource - Avoiding conflicts when multiple configurations manage different principals on the same object
|
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got through a pass. LMK what you think!
|
|
||
| -> This resource can only be used with a workspace-level provider! | ||
|
|
||
| ~> This resource is _authoritative_ for the specified object-principal pair. Configuring this resource will manage the permission for the specified principal only, without affecting permissions for other principals. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe good to note that you can't use both databricks_permission and databricks_permissions for the same object.
| * `group_name` - (Optional) Group name to grant permissions to. Conflicts with `user_name` and `service_principal_name`. | ||
| * `service_principal_name` - (Optional) Application ID of the service principal. Conflicts with `user_name` and `group_name`. | ||
|
|
||
| Exactly one of the following object identifiers must be specified: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious: do you like the pattern of having a separate field for each permission type? I was thinking that it might be more scalable to split a bit from how databricks_permissions works and instead have object_type and object_id fields. That way, as new object types are added, this resource doesn't need to be updated. I think the IDs are all the same type IIUC. We should then point people to the REST API docs for the set of allowed types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be easier to read in some cases, but let me try to play with it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we use specific field name to validate what are supported permissions... and additional customization. So it makes sense to support only explicit fields. Plus, compatibilit with permissions resouce
| // concurrently, each doing GET -> filter -> SET, which could result in | ||
| // lost permission updates. | ||
| type objectMutexManager struct { | ||
| mutexes map[string]*sync.Mutex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: you can use sync.Map and then don't need a separate mapLock.
| // lockObject acquires a lock for the given object ID. | ||
| // This should be called at the start of any operation that modifies permissions. | ||
| func lockObject(objectID string) { | ||
| globalObjectMutexManager.Lock(objectID) | ||
| } | ||
|
|
||
| // unlockObject releases the lock for the given object ID. | ||
| // This should be called at the end of any operation that modifies permissions. | ||
| // Use defer to ensure it's always called even if the operation panics. | ||
| func unlockObject(objectID string) { | ||
| globalObjectMutexManager.Unlock(objectID) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these used outside of tests? Do we need these?
| // Lock acquires a mutex for the given object ID. | ||
| // Each object ID gets its own mutex to allow concurrent operations on different objects | ||
| // while serializing operations on the same object. | ||
| func (m *objectMutexManager) Lock(objectID string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is just used for permissions, maybe we accept object type and object ID as parameters and synthesize an appropriate key.
| } | ||
|
|
||
| // Parse the ID to get object ID and principal | ||
| objectID, _, err := r.parseID(id.ValueString()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be necessary in update, since the object ID should be in the PermissionResourceModel
| resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("permission_level"), currentPermissionLevel)...) | ||
| } | ||
|
|
||
| func (r *PermissionResource) Update(ctx context.Context, req resource.UpdateRequest, resp *resource.UpdateResponse) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are create and update basically the same? (which makes sense because permissions are essentially a singleton) Should we deduplicate by putting the create and update implementations in a single method?
| var id types.String | ||
| resp.Diagnostics.Append(req.State.GetAttribute(ctx, path.Root("id"), &id)...) | ||
| if resp.Diagnostics.HasError() { | ||
| return | ||
| } | ||
|
|
||
| w, err := r.Client.WorkspaceClient() | ||
| if err != nil { | ||
| resp.Diagnostics.AddError("Failed to get workspace client", err.Error()) | ||
| return | ||
| } | ||
|
|
||
| // Parse the ID to get object ID and principal | ||
| objectID, principal, err := r.parseID(id.ValueString()) | ||
| if err != nil { | ||
| resp.Diagnostics.AddError("Failed to parse resource ID", err.Error()) | ||
| return | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto, we shouldn't need to parse the ID to get details about the permission, it should be in state. The only place it may not be in state is in Read (post-import).
| // Set principal fields - use null for empty strings to avoid ImportStateVerify failures | ||
| if userName != "" { | ||
| resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("user_name"), types.StringValue(userName))...) | ||
| } else { | ||
| resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("user_name"), types.StringNull())...) | ||
| } | ||
|
|
||
| if groupName != "" { | ||
| resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("group_name"), types.StringValue(groupName))...) | ||
| } else { | ||
| resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("group_name"), types.StringNull())...) | ||
| } | ||
|
|
||
| if servicePrincipalName != "" { | ||
| resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("service_principal_name"), types.StringValue(servicePrincipalName))...) | ||
| } else { | ||
| resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("service_principal_name"), types.StringNull())...) | ||
| } | ||
|
|
||
| // Set permission level | ||
| resp.Diagnostics.Append(resp.State.SetAttribute(ctx, path.Root("permission_level"), types.StringValue(permissionLevel))...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we populate an instance of PermissionResourceModel and then write the entire resource to the plan?
| @@ -0,0 +1,518 @@ | |||
| package permissions_test | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move this test to the internal/providers/pluginfw/products/permissions package?
Changes
This resource provides fine-grained control over permissions by managing a single principal's access to a single object, unlike
databricks_permissions, which manages all principals' access to an object at once. This is particularly useful for:Caveat: Since we cannot remove an individual permission, the
Deleteoperation is performed asRead/Put, so we need to use a lock around each object.Tests
make testrun locallydocs/folderinternal/acceptanceNEXT_CHANGELOG.mdfile